[local_auth] Fix iOS crash when no localizedReason #3780

enricobenedos · 2021-04-02T06:55:50Z

If you fill localizedReason with an empty string the app crash without any exception or log on iOS simulator. I've added an assertion and a related test in order to notify developer that the field cannot be null but also cannot be an empty string.

This PR should fix #77794.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

google-cla · 2021-04-02T06:55:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

enricobenedos · 2021-04-02T07:57:20Z

@googlebot I fixed it.

cyanglaz · 2021-04-02T18:14:22Z

packages/local_auth/lib/local_auth.dart

@@ -101,6 +101,8 @@ class LocalAuthentication {
    bool biometricOnly = false,
  }) async {
    assert(localizedReason != null);


Since this plugin has migrated to null safety, this line is actually not necessary anymore. Feel free to remove it while you are here.

cyanglaz · 2021-04-02T18:15:23Z

packages/local_auth/lib/local_auth.dart

@@ -101,6 +101,8 @@ class LocalAuthentication {
    bool biometricOnly = false,
  }) async {
    assert(localizedReason != null);
+    assert(localizedReason.isNotEmpty);


What's the behavior on android if this is empty?

On Android 11 (simulator) there are no problems if the localizedReason is empty. Is it a good idea to add the assert only on iOS case using Platform.isIOS?

Is it a good idea to add the assert only on iOS case using Platform.isIOS?

Yes. If android can handle null, especially if null and empty string means different things on android, we should allow null on android. Do you know if there are difference between null and empty string on android?

Looking at this again. I think you are right, we really shouldn't allow empty strings or null on android either. So the change looks good.

@enricobenedos Let's make sure to mention the localizedReason must not be an empty String in the documentation.

Hello @cyanglaz, sorry for the delay but I'm a little bit busy. I'm happy that you check that is it a good idea to also not give the possibility to input an empty string on Android. I will update the docs soon.

Awesome! Thanks, once you update the doc we will land this!

I've updated the authenticate method documentation.

cyanglaz

Almost ready! Just some nits :)

packages/local_auth/CHANGELOG.md

packages/local_auth/lib/local_auth.dart

Co-authored-by: Chris Yang <[email protected]>

enricobenedos · 2021-04-09T06:59:56Z

Thank you @cyanglaz, I've applied your suggestions 😃

cyanglaz

Awesome! LGTM

…lugins#3780)

…lugins#3780) (#80118)

* master: (397 commits) [in_app_purchase] Implementation of platform interface (flutter#3781) [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819) [tools] fix version check command not working for new packages (flutter#3818) [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795) fix MD (flutter#3815) Path provider windows crash fix (flutter#3814) [local_auth] docs update (flutter#3103) Update PULL_REQUEST_TEMPLATE.md (flutter#3801) [quick_actions] handle cold start on iOS correctly (flutter#3811) Replace path_provider_linux widget tests with simple unit tests (flutter#3812) [sensors] format dart code based on the new dart formatter (flutter#3809) [google_sign_in] Fix "pick account" on iOS (flutter#3805) [image_picker_platform_interface] Added pickMultiImage (flutter#3782) [in_app_purchase] Added currency code and numerical price to product detail model. (flutter#3794) [local_auth] Fix iOS crash when no localizedReason (flutter#3780) Fix and update version checks (flutter#3792) [in_app_purchase] Configured example app to use StoreKit Testing on iOS 14 (flutter#3772) [local_auth] Unnecessary reassignment in example removed (flutter#2983) [flutter_webview] Fix `allowsInlineMediaPlayback` ignored on iOS (flutter#3791) Switch script/tools over to the new analysis options (flutter#3777) ...

google-cla bot added the cla: no label Apr 2, 2021

github-actions bot added the p: local_auth label Apr 2, 2021

enricobenedos force-pushed the local_auth_app_crash branch from f2a2a34 to 0de36e1 Compare April 2, 2021 07:49

google-cla bot added cla: yes and removed cla: no labels Apr 2, 2021

enricobenedos added 2 commits April 2, 2021 09:56

Fix iOS crash when no localizedReason

25bead8

Updated versioning files

64db76c

enricobenedos force-pushed the local_auth_app_crash branch from 0de36e1 to 64db76c Compare April 2, 2021 07:56

cyanglaz reviewed Apr 2, 2021

View reviewed changes

cyanglaz added in review labels Apr 2, 2021

enricobenedos and others added 3 commits April 8, 2021 19:35

Removed null assertion due to null safety

52bfb1d

Updated authenticate() method documnetation

60c4e52

Merge branch 'master' into local_auth_app_crash

4f83842

cyanglaz reviewed Apr 8, 2021

View reviewed changes

packages/local_auth/CHANGELOG.md Outdated Show resolved Hide resolved

packages/local_auth/lib/local_auth.dart Outdated Show resolved Hide resolved

enricobenedos and others added 2 commits April 9, 2021 08:58

Update packages/local_auth/CHANGELOG.md

2f8a09a

Co-authored-by: Chris Yang <[email protected]>

Update packages/local_auth/lib/local_auth.dart

8e3fe6e

Co-authored-by: Chris Yang <[email protected]>

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 9, 2021

cyanglaz approved these changes Apr 9, 2021

View reviewed changes

fluttergithubbot merged commit 1dbc262 into flutter:master Apr 9, 2021

github-actions bot added the needs-publishing label Apr 9, 2021

engine-flutter-autoroll mentioned this pull request Apr 9, 2021

Roll Plugins from 89ccc0eadb2c to 1dbc262327e2 (1 revision) flutter/flutter#80118

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2021

1dbc262 [local_auth] Fix iOS crash when no localizedReason (flutter/p…

890e1e6

…lugins#3780)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Apr 9, 2021

1dbc262 [local_auth] Fix iOS crash when no localizedReason (flutter/p…

0f8a7cb

…lugins#3780) (#80118)

cyanglaz removed the needs-publishing label Apr 9, 2021

cyanglaz removed the last mile label Apr 20, 2021

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[local_auth] Fix iOS crash when no localizedReason (flutter#3780)

bc684e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[local_auth] Fix iOS crash when no localizedReason #3780

[local_auth] Fix iOS crash when no localizedReason #3780

enricobenedos commented Apr 2, 2021 •

edited

Loading

google-cla bot commented Apr 2, 2021

enricobenedos commented Apr 2, 2021

cyanglaz Apr 2, 2021

enricobenedos Apr 8, 2021

enricobenedos Apr 8, 2021

cyanglaz Apr 2, 2021

enricobenedos Apr 4, 2021

cyanglaz Apr 5, 2021

cyanglaz Apr 8, 2021

cyanglaz Apr 8, 2021

enricobenedos Apr 8, 2021

cyanglaz Apr 8, 2021

enricobenedos Apr 8, 2021

cyanglaz left a comment

enricobenedos commented Apr 9, 2021

cyanglaz left a comment

[local_auth] Fix iOS crash when no localizedReason #3780

[local_auth] Fix iOS crash when no localizedReason #3780

Conversation

enricobenedos commented Apr 2, 2021 • edited Loading

Pre-launch Checklist

google-cla bot commented Apr 2, 2021

enricobenedos commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

enricobenedos commented Apr 9, 2021

cyanglaz left a comment

Choose a reason for hiding this comment

enricobenedos commented Apr 2, 2021 •

edited

Loading